Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polish component docs #153

Merged
merged 44 commits into from
Jun 16, 2020
Merged

Polish component docs #153

merged 44 commits into from
Jun 16, 2020

Conversation

jerelmiller
Copy link
Contributor

@jerelmiller jerelmiller commented Jun 16, 2020

Description

Fully polishes the component documentation with various refactors to ensure consistency.

Reviewer Notes

Visit a component such as the <Button />: https://pr-153.dlyi50rq9kt6c.amplifyapp.com/components/button

Some components worth looking at:

  • /apis/nerdlet => Has constants
  • /components/table-header-cell => Has wrapping prop default value
  • /components/entity-count-query => Nested prop types
  • /components/nrql-query => Has type defs

Related Issue(s) / Ticket(s)

If there are any related GitHub Issues or JIRA tickets, add links to them here.

Screenshot(s)

Screen Shot 2020-06-15 at 10 33 35 PM

Screen Shot 2020-06-15 at 10 33 44 PM

Screen Shot 2020-06-15 at 10 34 18 PM

Screen Shot 2020-06-15 at 10 34 25 PM

@jerelmiller jerelmiller added the enhancement New feature or request label Jun 16, 2020
Copy link
Contributor

@djsauble djsauble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Component docs look good. I'm noticing a couple of spacing issues that are probably not related to this particular PR, but calling them out just in case:

  • Need more spacing between the global nav and the sidebar/content columns
  • Need more spacing + a vertical border between the sidebar and content columns

@LizBaker
Copy link
Contributor

i'm finding the nested prop types a little confusing to looks at, have we talked about having borders similar to one core? I also find the closing > of <One of> to be a bit odd when it's on a new line
Screen Shot 2020-06-16 at 1 44 31 PM
Screen Shot 2020-06-16 at 1 44 42 PM

@jerelmiller
Copy link
Contributor Author

I'm open to putting the borders between the different elements. I hesitated doing it only because I was on the fence about the short border when the prop type is nested.

Do you think it would be better to omit the <One of> if there are no values to inject between the brackets? It looks like maybe this is an omission in the documentation generated from the SDK. Personally I find it confusing to see the list empty.

Comment on lines +24 to +32
CodeDef.Block = Block;
CodeDef.Bracket = Bracket;
CodeDef.Comment = Comment;
CodeDef.Keyword = Keyword;
CodeDef.Identifier = Identifier;
CodeDef.Number = NumberValue;
CodeDef.Operator = Operator;
CodeDef.String = StringValue;
CodeDef.Type = Type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on doing this for the reuse/flexibility/consistency 💯

@timglaser timglaser merged commit 742a9eb into master Jun 16, 2020
@timglaser timglaser deleted the jerel/polish-component-docs branch June 16, 2020 21:02
@djsauble
Copy link
Contributor

@LizBaker I removed the borders in a previous PR, but you're right, the nested props get kind of crazy without them. What if we tried indenting the props the way they're indented in the One Core docs, to emphasize that they're grouped together more?

@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants